Anchor textbox autocomplete to the cursor#5021
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Ctrl‑N/P first‑responder routing for terminal textboxes, refactors mention query/candidate shapes and trigger-specific insertion text, implements async ripgrep-backed file indexing with directory-skip/coalescing, redesigns controller staleness tracking, migrates mention UI to an NSPanel host, and expands tests covering these behaviors. ChangesMention Completion Workflow Overhaul
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant TextBoxInputTextView
participant TextBoxMentionCompletionController
participant TextBoxMentionIndexStore
participant NSPanel
User->>TextBoxInputTextView: Type trigger (@, /, $) or press Ctrl-N/Ctrl-P
TextBoxInputTextView->>AppDelegate: keyEquivalent handling
AppDelegate->>TextBoxInputTextView: forward Ctrl-N/P to first responder when routed
TextBoxInputTextView->>TextBoxMentionCompletionController: refresh(query, root)
TextBoxMentionCompletionController->>TextBoxMentionIndexStore: load candidates async
alt File indexing path
TextBoxMentionIndexStore->>TextBoxMentionIndexStore: spawn `rg --files` stream or enumerate filesystem
TextBoxMentionIndexStore->>TextBoxMentionIndexStore: apply shouldSkipIndexedDirectoryName and budgets
end
TextBoxMentionIndexStore-->>TextBoxMentionCompletionController: return candidates (with targetPath)
TextBoxMentionCompletionController-->>TextBoxInputTextView: update suggestions state (current/stale, selection)
TextBoxInputTextView->>NSPanel: create/update panel positioned at query anchor
NSPanel-->>User: show suggestions
User->>TextBoxInputTextView: navigate (Ctrl-N/P) or accept/submit
TextBoxInputTextView->>TextBoxMentionCompletionController: accept or submit based on trigger and state
TextBoxInputTextView->>NSPanel: dismiss panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR refactors
Confidence Score: 4/5The core indexing and keyboard-routing changes are structurally sound, but the panel lifecycle has a regression where the floating completion window stays visible after the parent window loses key status to another in-app window. The NSPanel replacement for NSPopover does not dismiss the completion panel when didResignKeyNotification fires: repositionMentionCompletionPanelIfNeeded returns silently when parentWindow.isKeyWindow is false, leaving the panel floating over other application windows until a suggestion-state change triggers syncMentionCompletionPopover. The old NSPopover would have handled this automatically. Everything else — the async process bridging via TextBoxProcessTerminationStatus, the coalesced refresh task design, and the acceptance guards — is well-constructed. Sources/TextBoxInput.swift — specifically the repositionMentionCompletionPanelIfNeeded guard fall-through and the didResignKeyNotification handler that schedules a reposition instead of a dismiss. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (keystroke)
participant TV as TextBoxInputTextView
participant Ctrl as TextBoxMentionCompletionController
participant Store as TextBoxMentionIndexStore (actor)
participant Panel as TextBoxMentionCompletionPanel
User->>TV: keyDown / textDidChange
TV->>TV: refreshMentionCompletions()
TV->>Ctrl: refresh(for: query, rootDirectory:)
Ctrl->>Ctrl: cancel lookupTask, set isLoadingSuggestions
Ctrl->>Ctrl: onStateChanged()
TV->>Panel: syncMentionCompletionPopover() — show/resize
Ctrl-->>Store: "Task { await suggestions(for:rootDirectory:) }"
Store-->>Store: cachedFileIndex or refreshFileIndex
Note over Store: Task.detached → rg/FileManager scan
Store-->>Ctrl: [TextBoxMentionSuggestion]
Ctrl->>Ctrl: update suggestions, onStateChanged()
TV->>Panel: syncMentionCompletionPopover() — update rows
User->>TV: Up/Down or Ctrl-N/P
TV->>Ctrl: moveSelection(delta:)
Ctrl->>TV: onStateChanged()
TV->>Panel: syncMentionCompletionPopover() — scroll to row
User->>TV: Return / Tab
TV->>TV: acceptMentionCompletion()
TV->>TV: replaceCharacters(in: query.range)
TV->>Ctrl: clear()
Ctrl->>TV: onStateChanged()
TV->>Panel: dismissMentionCompletionPopoverOnly()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1809,27 +1994,32 @@ actor TextBoxMentionIndexStore { | |||
| includingPropertiesForKeys: [.isDirectoryKey, .isRegularFileKey], | |||
| options: [.skipsHiddenFiles, .skipsPackageDescendants], | |||
| errorHandler: { _, _ in true } | |||
| ) else { | |||
| return [] | |||
| } | |||
| ) else { return result } | |||
|
|
|||
There was a problem hiding this comment.
scanFilesWithRipgrep
stdoutHandle.readData(ofLength: 64 * 1024) is a synchronous blocking I/O call, and process.waitUntilExit() blocks until the subprocess exits. Both run inside Task.detached(priority: .utility), which puts them on the Swift cooperative thread pool. Blocking a cooperative pool thread with synchronous process I/O stalls the thread until rg finishes — on a large repo this can be seconds — and can starve other concurrency work. On macOS, the runtime can promote blocked threads but there is no guarantee, and sustained blocking under keystroke pressure degrades the whole app.
The correct shape is to bridge the pipe to async using a completion-handler-to-async wrapper (e.g. FileHandle.bytes / AsyncBytes, DispatchIO, or a withCheckedContinuation wrapping terminationHandler) so the cooperative thread is yielded while waiting for I/O, matching the async caller at the call site.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| if query.query.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return Self.scanRootFiles(rootURL: URL(fileURLWithPath: rootDirectory, isDirectory: true)) | ||
| .prefix(Self.suggestionLimit) | ||
| .map { $0.suggestion(trigger: query.trigger) } | ||
| } |
There was a problem hiding this comment.
scanRootFiles synchronous I/O holds the actor executor
Self.scanRootFiles(...) calls FileManager.default.contentsOfDirectory synchronously without being wrapped in a detached task, unlike the non-empty-query path which uses Task.detached. Because fileSuggestions is an actor method, this call blocks the TextBoxMentionIndexStore actor's executor thread for the duration of the directory stat. While root scans are typically fast, the pattern is inconsistent with the rest of the method and could add noticeable latency if the root is on a mounted network volume or a slow path. Wrapping the call in a Task.detached(priority: .utility) (same as the full scan) keeps the actor free to service concurrent requests.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| private static func subsequenceMatchPositions(query: String, in text: String) -> [Int] { | ||
| let q = Array(query.lowercased()) | ||
| let t = Array(text.lowercased()) | ||
| guard !q.isEmpty, !t.isEmpty else { return [] } | ||
| var positions: [Int] = [] | ||
| var qIdx = 0 | ||
| for (tIdx, char) in t.enumerated() { | ||
| if qIdx < q.count, char == q[qIdx] { | ||
| positions.append(tIdx) | ||
| qIdx += 1 | ||
| if qIdx == q.count { return positions } | ||
| } | ||
| } | ||
| return qIdx == q.count ? positions : [] |
There was a problem hiding this comment.
subsequenceMatchPositions converts both the query and text to Array(text.lowercased()) for case-insensitive matching and returns indices into that array. Those indices are then passed to title.index(title.startIndex, offsetBy: position, ...) on the original, non-lowercased title. For characters where lowercased() changes the number of Unicode extended grapheme clusters (e.g., the German sharp-S ß lowercases to ss — two grapheme clusters instead of one), positions in the lowercased array diverge from positions in the original string, causing highlights to land on the wrong characters or crash with an out-of-bounds offset. File paths are overwhelmingly ASCII so this rarely triggers, but the invariant is broken. Matching and collecting Range<String.Index> directly in the original string (using String.compare with .caseInsensitive) avoids the mismatch entirely.
| private func shouldBypassHiddenMentionCompletionKeyboardInteraction() -> Bool { | ||
| guard let window else { return false } | ||
| guard NSApp.isActive, | ||
| window.isKeyWindow, | ||
| window.firstResponder === self, | ||
| mentionCompletionPanel?.isVisible == true else { | ||
| dismissMentionCompletions() | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private func shouldBypassMentionCompletionKeyboardAcceptance() -> Bool { | ||
| guard let query = mentionCompletionController.activeQuery, | ||
| query.kind == .skill, | ||
| query.query.isEmpty else { | ||
| return false | ||
| } | ||
| dismissMentionCompletions() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Both shouldBypassHiddenMentionCompletionKeyboardInteraction() and shouldBypassMentionCompletionKeyboardAcceptance() are named like pure predicates but call dismissMentionCompletions() inside their guard path. Any future caller added during a refactor (e.g., a new key or command handler) will trigger an unconditional dismissal simply by checking the condition, with no indication at the call site that state is being mutated. Separating the dismiss call to the call site (i.e., if shouldBypass... { dismissMentionCompletions(); return false }) makes the mutation visible and prevents accidental double-dismiss if a caller path changes.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 7348-7360: The test currently asserts the first suggestion is the
temp fixture which is environment-dependent; change the assertions to search the
suggestions array for an entry whose title equals "/nested-skill" (using
TextBoxMentionIndexStore.shared.suggestions result and TextBoxMentionQuery) and
then assert that that found suggestion's insertionText
hasPrefix("[/nested-skill]("); in other words, replace
XCTAssertEqual(suggestions.first?.title, "/nested-skill") and the XCTAssertTrue
on suggestions.first with a contains/first(where:) lookup for title ==
"/nested-skill" and assert properties on that found item.
In `@Sources/TextBoxInput.swift`:
- Around line 5241-5258: The new TextBoxMentionCompletionPanel is created
without a stable cmux.* window identifier; set a deterministic
NSUserInterfaceItemIdentifier on the panel after creation (e.g., assign
panel.identifier = NSUserInterfaceItemIdentifier("cmux.mentionCompletionPanel")
or another stable cmux.* name) so global window routing/ownership rules remain
deterministic—apply this change where TextBoxMentionCompletionPanel is
instantiated and assigned to mentionCompletionPanel.
- Around line 1904-1911: The ripgrep glob changes aren’t necessary—keep the
existing skip globs as-is—and add a stable NSPanel identifier when creating the
mention panel: inside makeMentionCompletionPanel(host:) (where the
TextBoxMentionCompletionPanel is instantiated) set panel.identifier =
NSUserInterfaceItemIdentifier("cmux.textbox.mentionCompletionPanel") so the
panel has a stable cmux.* identifier for window/panel management.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1220e732-67ea-44d1-8240-47eb1c4c3fcb
📒 Files selected for processing (4)
Sources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TextBoxInput.swift (1)
5252-5268:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a stable
cmux.*identifier to the mention completion panel.Line 5252 creates a user-visible
NSPanelwithoutpanel.identifier, which violates the window identity rule and can break shared window routing/ownership behavior.Suggested fix
let panel = TextBoxMentionCompletionPanel( contentRect: NSRect(origin: .zero, size: host.fittingSize), styleMask: [.borderless, .nonactivatingPanel], backing: .buffered, defer: false ) + panel.identifier = NSUserInterfaceItemIdentifier("cmux.textbox.mentionCompletionPanel") panel.isFloatingPanel = true panel.hidesOnDeactivate = trueAs per coding guidelines: “User-visible NSWindow, NSPanel, NSWindowController, SwiftUI Window, or SwiftUI WindowGroup must have a stable cmux.* window identifier.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TextBoxInput.swift` around lines 5252 - 5268, The newly created TextBoxMentionCompletionPanel instance (assigned to local variable panel and stored in mentionCompletionPanel) needs a stable cmux.* window identifier; set panel.identifier to an NSUserInterfaceItemIdentifier with a constant string like "cmux.mentionCompletionPanel" (or another stable cmux.* name) immediately after creating the panel so the user-visible NSPanel has a persistent identity for shared window routing and ownership.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/TextBoxInput.swift`:
- Around line 5252-5268: The newly created TextBoxMentionCompletionPanel
instance (assigned to local variable panel and stored in mentionCompletionPanel)
needs a stable cmux.* window identifier; set panel.identifier to an
NSUserInterfaceItemIdentifier with a constant string like
"cmux.mentionCompletionPanel" (or another stable cmux.* name) immediately after
creating the panel so the user-visible NSPanel has a persistent identity for
shared window routing and ownership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba0564a5-c0ea-4972-b957-72461b216e97
📒 Files selected for processing (1)
Sources/TextBoxInput.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
7192-7194:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid asserting the first empty-query file suggestion.
This fixture now returns multiple valid empty-query candidates (
@Sources/,@README.md,@ZEmpty/, etc.), so pinningsuggestions.firstmakes the test brittle to ranking changes even when the behavior is still correct. Assert that@Sources/is present, then verify its metadata.Suggested change
- XCTAssertEqual(suggestions.first?.title, "`@Sources/`") - XCTAssertEqual(suggestions.first?.systemImageName, "folder") - XCTAssertTrue(suggestions.first?.insertionText.hasPrefix("[`@Sources/`](") == true) + let sourcesDirectory = suggestions.first { $0.title == "`@Sources/`" } + XCTAssertNotNil(sourcesDirectory) + XCTAssertEqual(sourcesDirectory?.systemImageName, "folder") + XCTAssertTrue(sourcesDirectory?.insertionText.hasPrefix("[`@Sources/`](") == true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 7192 - 7194, The test currently assumes suggestions.first is the "`@Sources/`" candidate which is brittle; instead locate the candidate by searching the suggestions array for an element whose title == "`@Sources/`" (e.g. using first(where: { $0.title == "`@Sources/`" })), assert that this found suggestion is non-nil, then verify its systemImageName == "folder" and that its insertionText.hasPrefix("[`@Sources/`](") is true; update assertions that reference suggestions.first to use the located suggestion variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TextBoxInput.swift`:
- Around line 2000-2004: The current refresh path does a second full tree walk
because scanFilesWithRipgrep performs its own BFS and git-ignore probes even
after scanDirectoryCandidateSeed produced directorySeed
(directorySeed.candidates and seenDirectoryRelativePaths); fix by reusing or
gating that seed: modify scanFilesWithRipgrep to accept an optional
directorySeed or directoryCandidates parameter and when a non-empty
directorySeed is provided, skip the separate BFS and git check-ignore phase and
instead derive directory rows directly from the ripgrep file stream (populate
directoryCandidates and update seenDirectoryRelativePaths from file paths
emitted by rg), or cache the computed directorySeed at a higher level and
early-return when it’s available so fileCandidates/reserveCapacity still happens
without a second traversal. Ensure changes reference scanFilesWithRipgrep,
scanDirectoryCandidateSeed, directorySeed, directoryCandidates, fileCandidates,
and seenDirectoryRelativePaths.
---
Duplicate comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 7192-7194: The test currently assumes suggestions.first is the
"`@Sources/`" candidate which is brittle; instead locate the candidate by
searching the suggestions array for an element whose title == "`@Sources/`" (e.g.
using first(where: { $0.title == "`@Sources/`" })), assert that this found
suggestion is non-nil, then verify its systemImageName == "folder" and that its
insertionText.hasPrefix("[`@Sources/`](") is true; update assertions that
reference suggestions.first to use the located suggestion variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc9a9f87-f26f-4ac3-9dbc-a4cbecbdea2f
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TextBoxInput.swift (2)
5468-5478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the bare
//$special-case scoped to Return.Line 5471 and Line 5506 route both Return and Tab through
shouldBypassMentionCompletionKeyboardAcceptance(), so an empty skill query now dismisses suggestions on Tab too. The PR only calls out Return as the submit path for bare/and$; Tab completion for root skill suggestions regresses here.Suggested fix
- private func shouldBypassMentionCompletionKeyboardAcceptance() -> Bool { + private func shouldBypassMentionCompletionReturnAcceptance() -> Bool { guard let query = mentionCompletionController.activeQuery, query.kind == .skill, query.query.isEmpty else { return false } dismissMentionCompletions() return true }case kVK_Return, kVK_ANSI_KeypadEnter: guard !flags.contains(.shift) else { return false } if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } + if shouldBypassMentionCompletionReturnAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion() case kVK_Tab: if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion()case `#selector`(NSResponder.insertNewline(_:)), - `#selector`(NSResponder.insertTab(_:)): + `#selector`(NSResponder.insertTab(_:)): + if commandSelector == `#selector`(NSResponder.insertNewline(_:)), + shouldBypassMentionCompletionReturnAcceptance() { + return false + } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion()Also applies to: 5503-5508, 5531-5539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TextBoxInput.swift` around lines 5468 - 5478, The Tab case is incorrectly sharing the same "bare `/` / `$` submit" bypass logic as Return; update the kVK_Tab branch so it does not call shouldBypassMentionCompletionKeyboardAcceptance() (or otherwise apply the bare-query bypass) — keep that special-case scoped only to the kVK_Return / kVK_ANSI_KeypadEnter branch; in practice modify the kVK_Tab handling around mentionCompletionController.hasStaleSuggestions and acceptMentionCompletion() to omit the call to shouldBypassMentionCompletionKeyboardAcceptance() so Tab will not dismiss root-skill suggestions while Return still uses the special-case check in shouldBypassMentionCompletionKeyboardAcceptance().
2080-2326:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBound
rg/gitsubprocess lifetime.
scanFilesWithRipgrep,isGitWorkTree, andgitIgnoredRelativePathsrunProcessand then block on completion (process.waitUntilExit()/ stdout reads) with no timeout or cancellation path. SincefileIndexRefreshTaskstores a detached scan infileIndexRefreshTasks[rootDirectory]andstoreFileIndexclears it only afterscanTask.valuereturns, a hungrg/gitcan leave the in-flight entry stuck and cause subsequent refreshes that await the index to block indefinitely. Add a timeout (terminating theProcess) and ensurefileIndexRefreshTasks[rootDirectory]is cleared on timeout/failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/TextBoxInput.swift`:
- Around line 5468-5478: The Tab case is incorrectly sharing the same "bare `/`
/ `$` submit" bypass logic as Return; update the kVK_Tab branch so it does not
call shouldBypassMentionCompletionKeyboardAcceptance() (or otherwise apply the
bare-query bypass) — keep that special-case scoped only to the kVK_Return /
kVK_ANSI_KeypadEnter branch; in practice modify the kVK_Tab handling around
mentionCompletionController.hasStaleSuggestions and acceptMentionCompletion() to
omit the call to shouldBypassMentionCompletionKeyboardAcceptance() so Tab will
not dismiss root-skill suggestions while Return still uses the special-case
check in shouldBypassMentionCompletionKeyboardAcceptance().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2217da3-70b8-4900-8d1d-c6489cb2c7e2
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TextBoxInput.swift (1)
1730-1741: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the new mention-completion pipeline into separate Swift files.
This PR adds filesystem/git/
rgindexing plus controller state to a file that is already far past the repo's size limit. Please split the new index store/controller out ofTextBoxInput.swiftbefore merge.As per coding guidelines: “Do not add more than 250 lines to an existing production Swift file that is already over 800 lines” and “Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file.”
Also applies to: 2585-2603
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TextBoxInput.swift` around lines 1730 - 1741, The TextBoxMentionIndexStore actor and its supporting types (e.g., TextBoxMentionIndexStore, CachedIndex, fileIndexTTL, maxCachedFileIndexes, directorySeedBatchSize and any helper methods used between the two highlighted ranges) must be extracted into one or more new Swift source files; move the entire actor, its nested structs/enums, constants, and any non-UI controller/state logic out of TextBoxInput.swift into a dedicated file (e.g., TextBoxMentionIndexStore.swift) and update visibility (public/internal/private) and imports so references from TextBoxInput.swift compile; ensure you also move or refactor any helper functions, extensions, and tests that reference these symbols, update import/module boundaries if needed, and run the build to fix any broken references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TextBoxInput.swift`:
- Around line 5542-5545: The Escape handling currently dismisses mention
completions whenever mentionCompletionController.shouldShowPopover is true, but
this is also true during the loading-only state; change the check so Escape only
dismisses when actual suggestion rows exist. In the block that calls
shouldBypassHiddenMentionCompletionKeyboardInteraction(), guard
mentionCompletionController.shouldShowPopover AND a check for visible results
(e.g., mentionCompletionController.hasRows or
mentionCompletionController.visibleResultsCount > 0) before calling
dismissMentionCompletions(); apply the same change to the analogous block at the
other occurrence (the block around the second instance that currently mirrors
lines 5571-5575).
- Around line 1858-1867: The code can evict the looked-up rootDirectory during
pruneFileIndexCache(now:), so first check fileIndexesByRoot for rootDirectory
and, if a valid CachedIndex exists (now.timeIntervalSince(cached.createdAt) <
Self.fileIndexTTL), update its lastAccessedAt immediately (write back a new
CachedIndex with cached.index and cached.createdAt but lastAccessedAt: now)
before calling pruneFileIndexCache(now:); alternatively, reorder to update the
cache entry's lastAccessedAt for rootDirectory prior to invoking
pruneFileIndexCache(now:), ensuring pruneFileIndexCache cannot evict the very
entry you're refreshing (references: fileIndexesByRoot,
pruneFileIndexCache(now:), CachedIndex, fileIndexTTL, rootDirectory).
---
Outside diff comments:
In `@Sources/TextBoxInput.swift`:
- Around line 1730-1741: The TextBoxMentionIndexStore actor and its supporting
types (e.g., TextBoxMentionIndexStore, CachedIndex, fileIndexTTL,
maxCachedFileIndexes, directorySeedBatchSize and any helper methods used between
the two highlighted ranges) must be extracted into one or more new Swift source
files; move the entire actor, its nested structs/enums, constants, and any
non-UI controller/state logic out of TextBoxInput.swift into a dedicated file
(e.g., TextBoxMentionIndexStore.swift) and update visibility
(public/internal/private) and imports so references from TextBoxInput.swift
compile; ensure you also move or refactor any helper functions, extensions, and
tests that reference these symbols, update import/module boundaries if needed,
and run the build to fix any broken references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87f2a3e8-8cda-477a-aaa5-46edf0d4fe3d
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } | ||
| guard mentionCompletionController.shouldShowPopover else { return false } | ||
| dismissMentionCompletions() | ||
| return true |
There was a problem hiding this comment.
Don't let Escape dismiss while only the loading spinner is visible.
shouldShowPopover is also true during the loading-only state, so Esc/cancel now clears the active query before any suggestion rows arrive. The new behavior contract here says Escape should dismiss only when rows exist.
Suggested fix
- guard mentionCompletionController.shouldShowPopover else { return false }
+ guard mentionCompletionController.hasSuggestions else { return false }
dismissMentionCompletions()
return true
...
- guard mentionCompletionController.shouldShowPopover else { return false }
+ guard mentionCompletionController.hasSuggestions else { return false }
dismissMentionCompletions()
return trueAlso applies to: 5571-5575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TextBoxInput.swift` around lines 5542 - 5545, The Escape handling
currently dismisses mention completions whenever
mentionCompletionController.shouldShowPopover is true, but this is also true
during the loading-only state; change the check so Escape only dismisses when
actual suggestion rows exist. In the block that calls
shouldBypassHiddenMentionCompletionKeyboardInteraction(), guard
mentionCompletionController.shouldShowPopover AND a check for visible results
(e.g., mentionCompletionController.hasRows or
mentionCompletionController.visibleResultsCount > 0) before calling
dismissMentionCompletions(); apply the same change to the analogous block at the
other occurrence (the block around the second instance that currently mirrors
lines 5571-5575).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TextBoxMentionMarkdown.swift">
<violation number="1" location="Sources/TextBoxMentionMarkdown.swift:16">
P2: Path escaping misses `<` in angle-wrapped markdown target. Raw `<` can break link parsing. Encode both `<` and `>`.</violation>
</file>
<file name="Sources/TextBoxMentionCompletionDetector.swift">
<violation number="1" location="Sources/TextBoxMentionCompletionDetector.swift:3">
P3: Namespace enum used as static container. Repo rule says no namespace-enums. Use a struct/type that is not enum-as-namespace.</violation>
</file>
<file name="Sources/TextBoxProcessTerminationStatus.swift">
<violation number="1" location="Sources/TextBoxProcessTerminationStatus.swift:16">
P2: Single continuation slot drops earlier waiters. Second `wait()` call can replace first and leave that task stuck. Store and resume all pending waiters.</violation>
</file>
<file name="Sources/App/ShortcutRoutingSupport.swift">
<violation number="1" location="Sources/App/ShortcutRoutingSupport.swift:213">
P2: Ctrl-N/Ctrl-P match uses ANSI keyCode only. Layout users can lose mention navigation routing. Match by normalized character too.</violation>
</file>
<file name="Sources/TextBoxMentionIndexStore.swift">
<violation number="1" location="Sources/TextBoxMentionIndexStore.swift:169">
P2: Cache timestamp uses request time, not store time. TTL can expire too early after slow scan. Use completion time when writing cache.</violation>
<violation number="2" location="Sources/TextBoxMentionIndexStore.swift:609">
P2: Process waits block runtime indexing path. This adds avoidable latency and fights async actor flow. Use async process completion/read pattern.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| private static func isGitWorkTree(rootURL: URL) -> Bool { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/env") | ||
| process.arguments = [ | ||
| "git", | ||
| "-C", rootURL.path, | ||
| "rev-parse", | ||
| "--is-inside-work-tree" | ||
| ] | ||
| process.standardOutput = FileHandle.nullDevice | ||
| process.standardError = FileHandle.nullDevice | ||
|
|
||
| do { | ||
| try process.run() | ||
| } catch { | ||
| return false | ||
| } | ||
| process.waitUntilExit() | ||
| return process.terminationStatus == 0 | ||
| } | ||
|
|
||
| private static func gitIgnoredRelativePaths(rootURL: URL, relativePaths: [String]) -> Set<String> { | ||
| guard !relativePaths.isEmpty else { return [] } | ||
|
|
||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/env") | ||
| process.arguments = [ | ||
| "git", | ||
| "-C", rootURL.path, | ||
| "check-ignore", | ||
| "--stdin" | ||
| ] | ||
|
|
||
| let stdin = Pipe() | ||
| let stdout = Pipe() | ||
| process.standardInput = stdin | ||
| process.standardOutput = stdout | ||
| process.standardError = FileHandle.nullDevice | ||
|
|
||
| do { | ||
| try process.run() | ||
| } catch { | ||
| return [] | ||
| } | ||
|
|
||
| let probePaths = relativePaths + relativePaths.map { "\($0)/" } | ||
| let input = probePaths.joined(separator: "\n") + "\n" | ||
| if let data = input.data(using: .utf8) { | ||
| stdin.fileHandleForWriting.write(data) | ||
| } | ||
| stdin.fileHandleForWriting.closeFile() | ||
|
|
||
| let output = stdout.fileHandleForReading.readDataToEndOfFile() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 || process.terminationStatus == 1, | ||
| let outputText = String(data: output, encoding: .utf8) else { | ||
| return [] | ||
| } | ||
|
|
||
| return Set(outputText | ||
| .split(separator: "\n", omittingEmptySubsequences: true) | ||
| .map(String.init)) | ||
| } |
There was a problem hiding this comment.
isGitWorkTree calls process.waitUntilExit() and gitIgnoredRelativePaths calls both readDataToEndOfFile() and process.waitUntilExit() synchronously. Both functions are called from two blocking contexts introduced in this PR:
scanRootFileSystemCandidates→ invoked synchronously from thefileSuggestionsactor method for every empty-query keystroke, meaningwaitUntilExit()(blocking for the duration of agit rev-parseorgit check-ignoresubprocess) runs directly on theTextBoxMentionIndexStoreactor's executor thread.scanDirectoryCandidateSeed→ called from inside theTask.detached(priority: .utility)body ofscanFilesWithRipgrep, blocking a cooperative thread pool thread until the git subprocess exits.
On a large repo or slow filesystem the git calls can take hundreds of milliseconds, stalling the actor for all concurrent suggestion lookups in case (1) and starving the cooperative thread pool in case (2). The previous blocking I/O in scanFilesWithRipgrep was fixed in this PR by switching to fileHandleForReading.bytes; the same async-via-terminationHandler/CheckedContinuation pattern from TextBoxProcessTerminationStatus should be applied here so the actor and cooperative threads are yielded while waiting for the git subprocesses.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 81f0e9a. Configure here.
1 participant
Summary
Testing
Not run: local XCTest/xcodebuild test actions per repo rule.
Note
Medium Risk
Touches global key routing for the textbox and async filesystem/git/rg indexing; behavior is scoped and heavily tested but regressions could affect typing shortcuts or suggestion quality on large trees.
Overview
Textbox
@///$autocomplete now uses a cursor-anchored, non-activating child panel (replacingNSPopover), with repositioning on window move/resize and a slimmer list UI (loading row, query highlighting, capped height).Keyboard behavior is tightened: Ctrl‑N / Ctrl‑P are forwarded to the textbox first responder (textbox-only, like arrows); mention navigation accepts Ctrl‑N/P/J/K; empty skill triggers (
/or$alone) submit on Return but Tab still accepts; stale or loading rows cannot be accepted; Escape only dismisses when the popover should show.Mention indexing is refactored into dedicated modules with a heavier
TextBoxMentionIndexStore: optional ripgrep scans, git ignore filtering, directory entries, package/bundle skips, coalesced background refreshes, warmup on root change, larger result limits, and$inserts plain skill tokens (not markdown links). Bare@can list root files while the full index loads.Tests expand coverage (new
TextBoxMentionCompletionTests); the mention panel is excluded from auxiliary Cmd+W lint rules.Reviewed by Cursor Bugbot for commit 47f2cc5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
$-triggered mention completion now accepts empty queries and inserts bare skill tokens;@//continue inserting link-style suggestionsImprovements
Tests